Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use target_link_libraries instead of ament_target_dependencies #973

Merged
merged 2 commits into from
Feb 14, 2025

Conversation

sloretz
Copy link
Contributor

@sloretz sloretz commented Feb 8, 2025

This shows one weakness of target_link_libraries: the target names are not automatically known. ament_cmake packages create a _TARGETS variable, so this CL makes the new ament_cmake package script use that. There might be cases where a new package depending on a plain cmake library would have worked before with ament_target_dependencies(), but does not work with this change.

I think using pure CMake here is worth it because we can point users to CMake docs instead of our own, but I'm open to feedback.

ament/ament_cmake#292

Copy link
Collaborator

@fujitatomoya fujitatomoya left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think using pure CMake here is worth it because we can point users to CMake docs instead of our own, but I'm open to feedback.

i think this is okay, but we have lots of documentation that recommends ament_target_dependencies e.g https://docs.ros.org/en/rolling/How-To-Guides/Ament-CMake-Documentation.html#linking-to-dependencies. those can also be updated in the future.

@ahcorde
Copy link
Contributor

ahcorde commented Feb 14, 2025

Pulls: #973
Gist: https://gist.githubusercontent.com/ahcorde/39b6009cbfdc2d70cd2e077bdcc0953a/raw/95f3d9b052978fef1bdb4c0ffa74d3b81d521875/ros2.repos
BUILD args: --packages-above-and-dependencies ros2lifecycle_test_fixtures ros2pkg
TEST args: --packages-above ros2lifecycle_test_fixtures ros2pkg
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/15192

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Linux-rhel Build Status
  • Windows Build Status

@ahcorde ahcorde merged commit 6aa2d98 into rolling Feb 14, 2025
3 checks passed
@ahcorde ahcorde deleted the sloretz__use_target_link_libraries branch February 14, 2025 14:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants